fix: resolve UK and Berlin Group consent access against the PSU that granted it - #2881
Open
hongwei1 wants to merge 69 commits into
Open
fix: resolve UK and Berlin Group consent access against the PSU that granted it#2881hongwei1 wants to merge 69 commits into
hongwei1 wants to merge 69 commits into
Conversation
… endpoints GET /aisp/balances and GET /aisp/transactions fetched every private account for the caller without validating the bearer token's UK consent binding first, unlike every sibling AISP endpoint (including their own single-account counterparts). A DirectLogin or OAuth2 token with no bound consent fell through into the account-fetch path and surfaced as a 500 Unknown Error instead of the expected 403 OBP-35035, and callers with real consent got real account data with no consent enforcement at all. Add the same checkUKConsent + passesPsd2Aisp guard already used by getAccountsAccountIdBalances and getAccountsAccountIdTransactions so all five real-data AISP endpoints share one consent contract. Also fix createTransactionsJsonNew to take the AccountId from the request path instead of deriving it from the first transaction's bank account, which returned a null AccountId whenever the account had no transactions.
# Conflicts: # obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala
…ubstitution Redirecting to > >(tee "$RUNTIME_LOG") makes the tee process inherit this script's own stdout. A caller that captures this script's output with out=$(./flushall_build_and_run.sh --background ...) never sees EOF, because the long-running server (and the tee backing the substitution) never exits on its own — the command substitution hangs forever even though the server started successfully. Redirect straight to the log file instead.
checkUKConsent no longer needs a live Hydra introspection endpoint — it checks the Bearer token's consent_id claim against an AUTHORISED consent. Two comments describing the balances/transactions scenarios still referred to the removed Hydra dependency; align them with the class doc above, which was already corrected during the merge from Simon/develop.
… v4.0.1 endpoints develop's Hydra removal (PR OpenBankProject#2866) rewrote ConsentUtil.checkUKConsent to read the consent_id claim directly off the Bearer access token instead of calling an external Hydra introspection endpoint. That was the only blocker keeping getAccounts/getAccountsAccountIdBalances/getAccountsAccountIdTransactions on a weak "authenticated -> not 401" assertion (mirroring the same limitation in UKOpenBankingV310AisTests for the equivalent v3.1 endpoints). The OAuth1-signed test requests carry no Bearer JWT, so the consent_id claim lookup now fails deterministically with 403 ConsentIdClaimMissing — asserted directly (status code + error message) instead of the previous placeholder.
…ent-403-assertions # Conflicts: # obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala
…tions' into develop-obp # Conflicts: # obp-api/src/test/scala/code/api/UKOpenBanking/v4_0_1/UKOpenBankingV401AccountInfoTests.scala
Neither flushall_build_and_run.sh nor flushall_fast_build_and_run.sh printed the port the server actually binds, only its PID. smoke_test.sh's start_and_test() parses "PID: <n>" and "PORT: <n>" from the script's captured output to know where to poll /root; with no PORT line it fails fast with "未打印实际监听端口" even after the server started and bound successfully, leaving the server running with nothing to verify it or clean it up. Print the actual dev.port value read from props/default.props (falling back to 8080) right after the PID line in both scripts' background-mode branch.
Matches the codebase-wide convention of bank-scoped params before account-scoped ones.
createUKConsentJWT wrote every granted permission as ConsentView(bank_id=null, account_id=null, view_id=permission) at consent-creation time, before any account is known. That row can never match a real account (User.hasAccountAccess does plain bank_id/account_id equality, no wildcard), so a UK consent's declared Permissions had no effect on what could actually be read — access depended entirely on unrelated pre-existing AccountAccess grants. Add Consent.grantUKConsentAccountAccess, called from authoriseUKConsent once the PSU has selected accounts (via a new account_ids field on the authorise request body), mirroring how updateViewsOfBerlinGroupConsentJWT resolves Berlin Group's IBAN-keyed access into real per-account grants. Since UK consents are exercised via an opaque OAuth2 Bearer token rather than BG's per-request Consent-JWT header, the AccountAccess rows are granted eagerly at authorisation time instead of re-derived per request. Add a regression test: a consent scoped to ReadAccountsBasic grants that view but leaves ReadBalances locked.
…iews The 6 UK Open Banking v4.0.1 system views (ReadAccountsBasic/Detail, ReadBalances, ReadTransactionsBasic/Debits/Detail) previously all shared the generic SYSTEM_VIEW_PERMISSION_COMMON set, so Detail granted nothing beyond Basic and a consent scoped to Balances alone still exposed full transaction and counterparty data. Berlin Group's ReadAccountsBerlinGroup and ReadBalancesBerlinGroup had zero ViewPermission rows at all (pure membership gating), unlike ReadTransactionsBerlinGroup which already has a full permission set. Add one can_* permission constant per view in constant.scala, mapped from the UK v4.0.1 spec's Detail Permissions table, and wire each into MapperViews.applyDefaultsForSystemView in place of the shared COMMON/untouched branches. accountant keeps SYSTEM_VIEW_PERMISSION_COMMON unchanged, split into its own case. Note for reviewers: existing deployments with these views already created at boot will have stale ViewPermission rows from the old COMMON set (or none, for the two BG views) — factoryResetSystemView needs to be re-run per view, or a migration added, to pick up the new defaults.
The UK v4.0.1 Permissions enum requires ReadTransactionsCredits as an independently-selectable code alongside ReadTransactionsDebits, but only Debits (and Basic/Detail) existed as a view constant. Add the missing view id and can_* permission set (shared with ReadTransactionsDebits and ReadTransactionsBasic, since direction is a query-parameter-shaped concern, not a field-visibility one), wire it into MapperViews.applyDefaultsForSystemView and the additional_system_views boot whitelist, and create it unconditionally in the sandbox data importer alongside the other UK/BG views for consistency. Direction filtering itself (returning only credit- or debit-direction transactions per which view the consent granted) is not wired into the transactions endpoint yet — documented as a follow-up in constant.scala, since it also depends on fixing a separate, pre-existing bug where JSONFactory_UKOpenBanking_401 always reports CreditDebitIndicator as "Credit" regardless of the actual transaction. Add exact permission-set regression coverage for all 6 UK + 2 BG system views to MappedViewsTest.
The sample.props.template's documented list of valid additional_system_views values had drifted from Boot.scala's actual whitelist: it was missing ReadBalancesBerlinGroup and ReadTransactionsBerlinGroup (already live for a while) and the newly-added ReadTransactionsCredits.
Not a bug fix — a design guard-rail. Berlin Group's frequencyPerDay, recurringIndicator, and validUntil, and UK's TransactionFromDateTime, ToDateTime, and ExpirationDateTime already live on the consent record (ConsentJWT), never as a view can_* permission. As the UK/BG system view permission sets get filled in, it's easy to reach for a can_* string to express a consent's time-boxing or access-frequency limit (e.g. a hypothetical can_see_transactions_last_90_days) — that conflates the two layers. Add a note at both ends (ConsentJWT and the SYSTEM_READ_*_VIEW_PERMISSION definitions) so future changes don't cross this boundary.
GET /obp/v5.1.0/user/current/consents/CONSENT_ID hard-required an exact mUserId match, so a PSU could never view a Berlin Group consent before completing SCA (BG consents are created via client_credentials with no owner yet). Relax the check to also allow consents that have no owner assigned. Also fix createStartConsentAuthorisationJson returning authenticationMethodId as the authorisationId instead of challengeId -- the PUT endpoint resolves the authorisation by challengeId, so any caller following the documented response field got a 400 on the confirmation step.
… end Create a PSU-less Berlin Group consent directly via the provider (mirroring how POST /consents builds one for a client_credentials caller), then exercise the full authorisation flow: another logged-in user can view it before SCA, starting the authorisation returns a resolvable authorisationId, and submitting the correct OTP claims the consent for the answering PSU. A second scenario documents that a wrong OTP is rejected with 400 and leaves the consent unclaimed.
UK v4.0.1 requires AWAU/AUTH/RJCT/CANC/EXPD on the wire, but OBP was returning the internal long enum names (AWAITINGAUTHORISATION, AUTHORISED, REVOKED) verbatim -- disagreeing with the endpoint's own documented example response, which already showed AWAU. Add a status-code mapping at the JSON serialization boundary only; internal storage keeps the long names, matching how BG and OBP standard consents already work. REVOKED maps to CANC since OBP does not otherwise distinguish PSU-dashboard-cancel from AISP-DELETE-revoke and the spec has no other status for that distinction. Also add the StatusReason array required by OBReadConsentResponse1, which was previously present only in the hardcoded ResourceDoc example, never actually populated in real responses. This is a breaking wire-format change for any caller currently depending on the long status-name strings.
ExpirationDateTime/TransactionFromDateTime/TransactionToDateTime are 0..1 per the UK spec's OBReadConsent1 (open-ended if absent), but the shared v3.1.0/v4.0.1 request body class declared them as required Strings -- omitting any one failed json4s extraction. Thread Option[Date] through saveUKConsent/createUKConsentJWT (mirrors the Option[Date] validUntil pattern createBerlinGroupConsentJWT already uses) so a missing ExpirationDateTime means the consent never expires -- represented as Long.MaxValue in the JWT's exp claim, not "now" (which is what the BG sibling function defaults to when its own validUntil is absent; that default is wrong for "no limit" and not copied here). Nothing currently reads this claim for UK consents (checkUKConsent doesn't check expiry yet), so this only sets up correct behaviour for that future gap. Also fix DateWithDayFormat's silent truncation: it's a bare "yyyy-MM-dd" SimpleDateFormat, so a full ISO-8601 datetime like "2020-01-01T00:00:00+00:00" parsed leniently and silently dropped the time and offset, and a malformed value threw an uncaught ParseException that surfaced as 500 instead of 400. Add parseIso8601OrDayDate (tries full ISO-8601 first, falls back to a bare date) and route it through NewStyle.function.tryons so malformed input reaches 400 -- a bare Future.fromTry(Try(...)) doesn't get mapped to 400 by ErrorResponseConverter, since it only preserves the code from APIFailureNewStyle. Both v3.1.0 and v4.0.1 create/get handlers updated (the request body class and parser are shared); GET responses null-guard the now-possibly-absent stored dates instead of stringifying a null Date.
An AUTHORISED UK consent was effectively perpetual: checkUKConsent only checked not-before (creationDateTime), never the consent's own ExpirationDateTime or the JWT exp, and ConsentScheduler's expiry tasks filtered on apiStandard=BG / apiStandard=obp, excluding UK entirely. A PSU who consented to "until date X" kept granting access indefinitely unless the consent was manually revoked -- a minimum-necessary-access / data-minimisation problem. Add both layers: - Reactive (the actual security control): checkUKConsent now rejects an AUTHORISED consent whose ExpirationDateTime has passed, with the existing OBP-35003 ConsentExpiredIssue (401). A null ExpirationDateTime means the consent never expires (0..1 per spec, open-ended if absent) and is skipped. This closes the gap immediately regardless of scheduler cadence. - Proactive: a new ConsentScheduler.expiredUKConsents task flips long-past-expiry AUTHORISED UK consents to EXPIRED so the stored status stays accurate for GET/dashboard reads. Interval prop uk_open_banking_expired_consents_interval_in_seconds (default 601, 0 to disable), mirroring the existing BG/OBP expiry tasks. A null mExpirationDateTime never matches By_< against a Date, so perpetual consents are correctly never selected.
authoriseUKConsentChallenge and authoriseUKConsent hard-guarded status == AWAITINGAUTHORISATION, so once a consent was AUTHORISED nothing could reopen it -- a TPP whose access token was lost had to create a brand-new consent. The UK spec allows re-authentication of the same ConsentId while its status is AUTH or CANC (OBP's REVOKED) and its ExpirationDateTime has not elapsed. Relax both endpoints' status guard to ukReAuthableStatuses (AWAITINGAUTHORISATION, AUTHORISED, REVOKED); EXPIRED and REJECTED stay terminal. Add two guards that the previous single-status check made unnecessary but re-auth now requires: - not-expired: reject re-auth of a consent past its ExpirationDateTime (null = never expires), consistent with the reactive expiry check added to checkUKConsent. - same-PSU: a consent already bound to a user may only be re-authorised by that same user, so a different user of the same consumer cannot hijack it (updateConsentUser rebinds mUserId unconditionally). Re-running the SCA + grantUKConsentAccountAccess flow is already idempotent (grantAccessToViews revokes-then-regrants per view), so a second authorise cannot double-grant. Tests cover the two new rejection guards at the challenge-start step. The successful re-auth happy path needs a completed SCA/OTP ceremony, which has no test harness in this repo yet (these authorise endpoints shipped without coverage), so it is not asserted here.
POST /aisp/account-access-consents (both v3.1.0 and v4.0.1) hard-failed with AuthenticatedUserIsRequired whenever no PSU was present, so a TPP could not lodge a consent with a client-credentials (app-only) token -- contradicting the UK spec's Step 2 (the TPP creates the consent as an app; the PSU authorises it later) and OBP's own doc comment on authoriseUKConsent, which describes exactly that client-credentials lodging model. Relax the check to reject only a fully anonymous request (no consumer and no user); a consumer-only context now lodges the consent with no bound PSU (mUserId stays null until authoriseUKConsent binds it after SCA), mirroring the Berlin Group native consent flow. saveUKConsent already takes user: Option[User], so a request that does carry a user (e.g. DirectLogin) is unchanged -- createdByUser just carries it through. The consent row grants nothing until authorised, so this does not widen data access; it only lets the spec-correct app-only lodging step work. The existing "authenticated -> 201" and "unauthenticated -> 401" scenarios still pass (401 now means fully-anonymous rather than no-PSU). The pure consumer-only path can't be exercised via the DirectLogin-based test harness (DirectLogin always binds a user), so it isn't asserted directly here.
No UK v4.0.1 response carried x-fapi-interaction-id. The only related mechanism was a generic, opt-in, empty-by-default header-mirror prop that echoes a request header verbatim if present -- it never generated one when the TPP omitted it, so interaction tracing didn't work for the common case. (An earlier plan iteration pointed at getHeadersNewStyle for this, but that path is dead Lift-era code never called from the http4s stack.) Wrap the UK v4.0.1 aggregator's routes in an outer middleware that sets x-fapi-interaction-id on every response: echoed from the request when the TPP supplies one, otherwise a freshly generated UUID. Applied outside ResourceDocMiddleware so it also covers error responses. Does not introduce x-fapi-financial-id (correctly absent since v3.x).
FAPI 1.0 Advanced needs a place to resolve a client's public key when verifying signed request objects and private_key_jwt client assertions. Adds a jwksUri Mapper field to Consumer (auto-migrates, same pattern as the existing unused clientCertificate field) and exposes it as jwks_uri through both the read-only and admin OIDC consumer views.
FAPI 1.0 Advanced's tls_client_auth needs the client's registered certificate at token-request time, not just in the admin view. The Consumer.clientCertificate field already exists (unused); this just adds it to v_oidc_clients alongside the existing jwks_uri column.
The merge from origin/develop pulled in createConsentWithStandard, which called saveUKConsent with raw java.util.Date arguments. This session's earlier work changed those three params to Option[Date], so the merge produced a type mismatch that git's line-based merge couldn't detect on its own — it only surfaces at compile time.
…ot hold grantUKConsentAccountAccess only verified that the requested account_ids exist (checkBankAccountExists), never that the PSU authorising the consent actually holds them. Any authenticated user could therefore authorise a UK consent naming an arbitrary existing account_id and be granted every consented view on it. Add an ownership check against AccountHolders.getAccountsHeld before binding the consent's views to the requested accounts; reject with a new OBP-35037 error when any requested account is not held by the current user. Add a regression test: resourceUser2 attempting to authorise a consent naming resourceUser1's account is rejected and gains no AccountAccess.
A single stale AccountHolder grant pointing at a deleted account made getCoreBankAccountsLegacy throw and 500 the entire "list my accounts" call, because it unconditionally opened each resolved account with openOrThrowException. Mirror the same tolerance already applied to the sibling method getBankAccounts: skip accounts that can't be resolved instead of failing the whole batch.
A client-credentials token still resolves cc.user to an auto-vivified pseudo-user (idGivenByProvider equal to the calling consumer's own client key) rather than leaving it Empty. Both UK consent creation endpoints (v3.1.0 and v4.0.1) carried that value straight through into saveUKConsent's user param, permanently binding the consent to the TPP's own pseudo-identity instead of leaving it unowned as intended. Since authoriseUKConsent/authoriseUKConsentChallenge reject authorisation unless the consent's userId is blank or matches the authorising user, a consent lodged this way could never be authorised by the real PSU -- every attempt failed with ConsentDoesNotMatchUser. Filter out that pseudo-user before passing it to saveUKConsent, so the consent stays unowned until the PSU actually authorises it.
The Account and Transaction API profile lists permission combinations an ASPSP
must reject with a 400 response code. Both UK account-access-consent endpoints
accepted all of them, so a consent could be lodged that no AISP can ever use:
every AIS endpoint other than /accounts is /accounts/{AccountId}/..., so a
consent naming no account-read permission can never discover the ids it would
need. Such a consent authorises normally and then returns an empty account list
forever, with nothing to explain why.
Add Consent.validateUKConsentPermissions covering the profile's rules -- a
non-empty array, at least one of ReadAccountsBasic/ReadAccountsDetail, and
transaction depth and direction each requiring the other -- and call it before
saving the consent in the v3.1 and v4.0.1 handlers.
Two rules are deliberately not enforced. "A permission code not supported by
the ASPSP" is about the endpoint subset an ASPSP publishes, and OBP publishes
no such list, so rejecting on it would be guesswork; a code that is not a UK
permission code at all is still refused. Requesting both a Basic and its
Detail counterpart stays allowed, because the profile calls that duplication
but forbids rejecting on that basis alone.
grantUKConsentAccountAccess purged stale UK permission views only on the accounts the consent being authorised names. The one path that most needs purging is the one it skipped: when the PSU re-authorises and drops an account from the selection, nothing ever clears the rows the previous, wider consent left on that account. AccountAccess rows carry no consent identity, and User.hasAccountAccess only asks whether a (user, account, view, consumer) row exists -- never whether the account belongs to the consent presented on this request. So those leftovers keep answering for the new consent: a consent declaring one account was observed returning two from GET /aisp/accounts, with 200 on the details and transactions of the account it never declared. Sweep every account the PSU holds at this bank instead, and on an account this consent does not name revoke all seven UK permission views rather than only the undeclared ones. The grant pass still only touches the accounts the consent names, so nothing it is entitled to is lost. Cost: two live consents held by the same TPP for the same PSU now trim each other on the accounts they do not share. That is the limitation AccountAccess already had within a single account -- latest authorisation wins -- now applied across accounts, and under-granting is the right side to err on for a consent-scope check. The complete fix is a consent_id column on AccountAccess, tracked separately. Rejected: filtering at read time in hasAccountAccess. It would need the request's consent threaded through every call site and would still leave the stale rows in the table for any path that does not carry one.
Narrowing a UK consent by account now holds when the PSU re-authorises, but not when the same TPP keeps a wider consent alive alongside the narrower one: both resolve to the same (user, account, view, consumer) rows, so re-authorising the wider one re-grants an account the narrower one never named. That limitation lived only in a status file, where nothing checks it. Pin it as a characterisation test instead, asserting what the code does today rather than what it should do, with the reasoning in the comments. When AccountAccess carries a consent_id the assertion becomes false and this scenario fails -- that failure is the signal to flip it, which is the whole point of writing it down here.
A UK consent's access was written to AccountAccess rows keyed on the real PSU. Those rows carry no consent identity, so every consent that PSU had granted shared one set of them and rewrote each other: authorising a second consent narrowed the first, and a consent could read accounts it had never named. The sweep added to contain that only made the shared rows tidier -- it could not make them belong to anyone. Berlin Group and OBP-native never had the problem. Their consent JWTs carry a random UUID in `sub`, which applyConsentRules resolves to a user that exists only for that consent and grants the JWT's views to. UK consents have always carried the same random `sub`; it was simply never used. Use it. The isolation stops being something to enforce and becomes what the data model says: one consent, one principal, its own rows. Both credential paths resolve it. The Consent-Id header path does so in applyUKRules; the Bearer path -- where the request is authenticated as the PSU long before checkUKConsent runs -- does so in a hook at the end of authentication that can only narrow the principal, never widen it, and falls back to today's behaviour on anything unexpected. The PSU is kept on the CallContext as `consenter`, and checkUKConsent's ownership check now compares against that, so "only the PSU this consent belongs to may use it" is unchanged. Three things follow, and are the reason this is worth the churn: The bulk endpoints stop leaking. GET /aisp/balances and its v3.1 twin list accounts with a bare `WHERE user_id = ?` and never check a view, so they answered for every account the PSU held whatever the consent said. The query needs no consent awareness now that the identity has it. The missing ReadBalances check is added anyway, and the two bulk /transactions endpoints move off the owner view, which a principal that owns nothing cannot hold -- and whose absence getOrElse(Nil) would have turned into a silent empty result. Firehose and ABAC stop applying. Both are consulted before the AccountAccess lookup and both key on entitlements; a UK consent JWT carries none, so its principal has none. No new guard on the shared access-control path was needed to get this. Revocation starts meaning something. It used to flip a status column and leave the granted rows live in the table forever. Rows now belong to exactly one consent, so revoke and expiry can drop them. grantUKConsentAccountAccess is left holding the account-holdership check and the JWT rewrite -- it grants nothing, and must keep running as the real PSU, since only a PSU holds accounts. grantAccessToViews reconciles instead of revoking and re-granting: running per request, the old shape let one request delete the row another had just written and 403 itself, or collide on the unique index. That was a live defect for Berlin Group too. Costs, stated plainly. Consent traffic now records the human on metric rows rather than the principal, which changes what Berlin Group and OBP-native rows have always held (a per-consent UUID and an empty username) -- an improvement, but a change. Consent principals are hidden from GET /users, where they had no business being. And a UK consent authorised before account binding existed still carries placeholder views naming no account; it keeps running as the PSU, with a warning, rather than silently losing all access.
grantAccessToViews runs on every request that presents a consent. The reconcile already meant a steady-state request wrote nothing, but it still resolved every consented view to decide it had nothing to do -- two DB lookups per view per request on a consent that had been used before, which is the common case. Filter the already-granted views out before the loop instead, so that request does no lookups either.
The scoping suite drives applyUKRules, which is the Consent-Id header path. UK Open Banking specifies the other one: the consent travels as a consent_id claim inside the OAuth2 access token, and the principal is swapped at the end of authentication rather than during it. That path had no coverage at all. Pin it with the same self-signed-JWT harness the expiry test already uses: the principal becomes the consent's, the PSU survives on the CallContext (checkUKConsent's ownership check and the CBS adapter both read it), the scope is the consent's and nothing more, and a token carrying no consent claim comes back untouched.
… paging Hiding them with a filter over the query result meant the limit/offset had already been applied, so a page containing consent principals came back short -- the same defect the ?locked= path in this method has. Push it into the WHERE clause instead. Both shapes the column takes are covered: NULL for rows written before it existed, empty for anyone not minted by a consent.
…ipal The shadow user is created lazily, on the first request that presents the consent -- so the first two requests race: both look, both find nothing, both insert, and one loses on the unique index over (provider, providerId). LiftUsers.getOrCreateUserByProviderId already handles exactly this by re-reading after a failed insert. It just was not on the Users trait, because only the Future form had been needed. Put it there and use it, rather than repeating the find-then-insert by hand.
* fix: stamp the build with the working copy that produced it git.properties came from git-commit-id-maven-plugin, and in a git worktree it described the wrong repository. The plugin bundles JGit 6.7, which has no commondir support and so cannot read a linked worktree's gitdir; its GitDirLocator.resolveWorktree() works around that by redirecting <main>/.git/worktrees/<name> back to <main>/.git. Every build run from a worktree therefore stamped the main checkout's branch and commit, and /status named a revision that never produced the running jar -- the failure commit 6727bd3 set out to make visible. Its PropertiesFileGenerator then skipped rewriting whenever only git.build.time differed, so the timestamp froze at the first build too. Generate the stamp with scripts/write_git_properties.sh instead, invoked from obp-api's maven-antrun execution at generate-resources. The git CLI is worktree-aware by construction and the script rewrites unconditionally. It writes straight into target/classes rather than the source tree, and only for obp-api: the plugin was declared in the parent pom as well, which put a second git.properties on the runtime classpath where whichever one /status read was incidental. Missing git or no repository yields git.commit.id=unknown rather than a failed build, matching the old failOnNoGitDirectory=false. Report git_branch and git_build_time on /status next to the commit, since a wrong branch was the symptom and neither was previously visible. test_worktree_build.yml only asserted the fields were non-empty, which the wrong values satisfied. It now compares them against the worktree's own HEAD and branch, rebuilds without clean at a new commit to prove the stamp moves, and checks the jar ships it. * test: pin /status to the build stamp the artifact carries Nothing covered the status page, which is the thing an operator reads to tell which build is running -- so the branch it reported being the main checkout's rather than the worktree's went unnoticed. Assert git_commit matches what APIUtil reads from the same classpath stamp, and that the branch, dirty flag and build time are all present. * ci: run the worktree build check on JDK 25 It requested JDK 11 while the build compiles with -release 25, so the job could never get past compilation -- which is why the stamp it is meant to guard went wrong unnoticed.
… access
anonymousAccess and applicationAccess opened with the same four stages copied
verbatim — resolve the user and session, verify the signed request, run the
Berlin Group checks, apply rate limiting — down to the variable names and the
rate_limiting.exclude_endpoints default. Extract them into accessPipeline so
each method is left with only the part that is actually its own, and fold the
four repeated url/verb/body/reqHeaders locals into requestPartsOf.
No behaviour change. The two terminal steps are preserved exactly:
anonymousAccess still runs the afterAuthenticateInterceptResult step and turns
a Failure into a 401 that keeps the original message, while applicationAccess
still answers ApplicationNotIdentified for anything that is not "no error and
a known Consumer".
applicationAccess is deliberately NOT expressed as anonymousAccess(cc) map {...}.
Stacking them would change behaviour twice over: anonymousAccess terminates a
Failure through fullBoxOrException, which throws, so an outer map never runs and
applicationAccess would lose its chance to answer ApplicationNotIdentified; and
app-mode callers would additionally inherit the intercept step, which is
specific to the anonymous path. The two tails are genuinely different decisions
about the same pipeline output, not one refining the other.
Its terminal match now spells out all three cases instead of letting Full fall
into the catch-all. That was correct only because Box.~> is identity on Full and
fullBoxOrException forwards Full untouched — two details a hundred lines apart
that the code should not have to depend on.
Verified with the full local suite on JDK 25: 4 shards, 374 suites, 0 failures
and 0 errors, including every suite that asserts ApplicationNotIdentified
(v5_0_0.ConsentRequestTest, v5_1_0.ConsumerTest, v5_1_0.VRPConsentRequestTest,
v6_0_0.DynamicEntityTest, v6_0_0.EndpointAuthModeTest, v6_0_0.GetOidcClientTest,
v6_0_0.VerifyOidcClientTest) and the ATM endpoints that pick between the two
methods in a single expression.
…pseudo-user A client-credentials token still resolves cc.user to an auto-vivified pseudo-user rather than leaving it Empty: OAuth2.getOrCreateResourceUser maps the JWT's `sub` onto idGivenByProvider, and in a client-credentials grant `sub` is the caller's own client id. POST /berlin-group/v1.3/consents carried that value straight through into createBerlinGroupConsent's user param, so the consent was owned by the TPP's own pseudo-identity instead of being left unowned until the PSU authorises it. That owner is neither blank nor the PSU, so both disjuncts of the guard on GET /obp/v5.1.0/user/current/consents/CONSENT_ID fail and the PSU is told OBP-35001 Consent not found for a consent that exists and is theirs to approve -- which is where the Berlin Group redirect-SCA journey has been stopping. The blank-owner branch added for unclaimed consents cannot help, because a pseudo-user owner is not blank. Leaving the owner unset restores the intended sequence: the consent is lodged unowned, the PSU can see it at SCA time, and the native authorisations endpoint pair binds them on a correct OTP. Nothing downstream needs the user at creation -- createBerlinGroupConsentJWT gives every consent a random `sub` regardless, which is what applyConsentRules resolves into the per-consent principal, and the user param only feeds the createdByUserId claim and an auth-context copy that the authorisation step writes again. This is the same defect and the same fix the UK endpoints took in def2da7; the Berlin Group creation path was missed at the time. The filter is copied rather than extracted into a shared helper on purpose: the UK sites and ConsentUtil are being edited on another branch, and a shared helper would collide there for no behavioural gain. The regression test drives POST /consents on a session whose user is keyed on the consumer's own client key, reproducing the client-credentials CallContext the OAuth1 test harness cannot mint directly. It was confirmed failing before the fix ("false was not true" on the unowned assertion) and passes after; a second scenario pins that a genuine PSU session is still recorded as the owner.
* fix: declare the auth mode UK consent lodging actually uses Lodging an account-access consent is a client-credentials call: the TPP is authenticated as an application and no PSU exists yet -- the PSU is bound later, during the authorise ceremony. Both UK handlers already say so, rejecting only a fully anonymous request rather than demanding a user. Their ResourceDocs did not. With no authMode they take the UserOnly default, which sends ResourceDocMiddleware down anonymousAccess, and that returns 401 for any request carrying no user. Nothing breaks today only because of a separate defect: OAuth2 token parsing resolves `sub` to a ResourceUser without asking what kind of token it came from, and for a client-credentials grant `sub` is the client_id (RFC 9068). The caller is handed an auto-vivified user that is not a person, cc.user is never Empty, and the UserOnly path never fires. The two hand-written guards inside these handlers -- the anti-anonymous check, and the filterNot that stops that pseudo-user becoming the consent's owner -- exist for the same reason and stay until the token defect is fixed. This is the first of three steps and deliberately the one that lands first: fixing the token defect before the contract is stated would make these endpoints 401 the very flow the standard requires them to serve. Behaviour is unchanged on every path that reaches them today. isAppMode only chooses applicationAccess over anonymousAccess; both call getUserAndSessionContextFuture first, so the consent principal hook at the end of authentication still runs, and both pass a Full user through untouched. The case that changes is one that cannot occur yet: a valid consumer with no user now reaches the handler instead of being rejected. That is the whole point -- it is the safety net for step three. One difference is real and worth stating plainly: a fully anonymous request still gets 401, but reports ApplicationNotIdentified rather than AuthenticatedUserIsRequired, and the ResourceDoc gains that error plus a note describing the auth mode. The Berlin Group twin, Http4sBGv13AIS.createConsent, has always behaved this way; UK now matches it. Both docs get a scenario pinning the auth mode, because nothing else would catch a silent revert to the default -- the endpoints would keep working right up until the day the token defect is fixed. * fix: hide consent principals from the v6.0.0 user search too The filter that keeps a consent's own principal out of GET /users was added to LiftUsers.getUsersCommon, which backs the v2.1.0 and v3.0.0 list endpoints. The v6.0.0 search never goes through it -- getUsersV600F builds its own query in DoobieUserQueries -- so it still returned one row per consent ever granted, each with no username and no email. Same predicate, and in the WHERE clause rather than over the result, so it composes with LIMIT/OFFSET instead of returning short pages. Users auto-vivified for an application are deliberately left visible. They look similar -- no real name, one row that stands for something other than a person -- but the reason for hiding consent principals was volume, one per consent granted, and that does not carry over: there is at most one per application. Against that, GET /users is role-gated and often read precisely to audit who holds access, and these rows can carry entitlements like any other. Hiding them would trade a little noise for an audit blind spot, and would also erase the only readily visible trace of the token-parsing defect that mints them. They should stop being created, not stop being shown.
Ports and the H2 database are already isolated per run, but Redis is not:
every checkout on a developer machine talks to the same 127.0.0.1:6379.
OBP_API_INSTANCE_ID feeds Constant.getGlobalCacheNamespacePrefix, which
prefixes every cache key with "{api_instance_id}_{runmode}_". The runner set
it to "shard_${n}", which is identical in every checkout, so two concurrent
runs shared one namespace. That matters because
LocalMappedConnectorTestSetup.wipeTestData deletes the whole namespace by
prefix after EVERY test: one run's teardown was deleting another run's live
rate-limit counters, once per test. Nothing failed, because the rate-limit
suites seed their counters immediately before asserting -- but that is luck,
not isolation.
Mixing in the already-allocated random port makes the namespace unique per
run, so a teardown only ever deletes its own keys.
Measured on a shared 127.0.0.1:6379 by sampling live keys:
before 4 prefixes shard_1_test_ .. shard_4_test_ (shared by all runs)
after 8 prefixes shard_1_26053_test_, shard_1_29629_test_, ...
all 8 observed coexisting in one snapshot, disjoint
Regression: run_tests_parallel.sh 3271 tests, 0 failures, 0 errors, both
standalone and with two checkouts running the full suite concurrently.
No production code is touched, and CI is unaffected: the workflows never set
OBP_API_INSTANCE_ID, so they keep using the "1_final" default from
ServerSetup. Nothing in the tree depends on the "shard_N" shape -- no test,
log parser or script matches on it.
This does not address cross-checkout resource-doc cache sharing: those keys
are also deterministic, but that hazard was not reproducible in testing
(wipeTestData clears them after every test, leaving only a very narrow
window) and is tracked separately.
…#59) The product caches in Caching already route through tryGet/trySet, whose comment states the intent plainly: if Redis is unreachable, treat it as a miss and recompute instead of failing the whole request. The resource-doc and swagger caches sit directly above them in the same file and did not -- they called Redis.use bare, and Redis.use throws rather than returning None. A Redis blip therefore turned every /resource-docs, /swagger, OpenAPI and message-docs request into a 500. These are exactly the documents API Explorer and Portal load on startup, so the blast radius is the whole developer-facing surface. Route all four get/set pairs through the same wrappers and give them explicit return types matching the product caches (Option[String] / Unit). Every call site already discarded the set return value, so narrowing it to Unit is source-compatible. Measured by running the suite against a dead Redis port (OBP_CACHE_REDIS_PORT=6399), which makes every Redis call fail: before 3273 tests, 135 failures after 3273 tests, 39 failures The 96 that disappear are the ones served from these caches: ResourceDocsTest (55), V7ResourceDocsAggregationTest (12), SwaggerDocsTest (11), MessageDocsJsonSchemaTest (8), DynamicEndpointsTest (6), ResourceDocsTechnologyTest (2), GetMessageDocsSwaggerTest (1) and Http4sServerIntegrationTest (1). With a healthy Redis the suite is unchanged: 3273 tests, 0 failures. The remaining 39 are other Redis dependencies, untouched here: rate limiting (23), the endpoints that exist to inspect Redis itself (12), and four consent/dauth scenarios that assert on an error message rather than a status code -- those still reject the request correctly, just with different wording. Rate limiting is unaffected in another way worth stating: RateLimitingUtil calls Redis.use directly rather than going through Caching, so the "REDIS_UNAVAILABLE" branch in getCounterState stays dead code. Making it live would mean changing Redis.use itself, which every caller shares.
The standard is explicit about who calls these. In the Endpoints table of account-access-consents, in both v3.1 and v4.0.1, all three consent endpoints carry Grant Type "Client Credentials", and the prose repeats it for each of GET and DELETE: "Prior to calling the API, the AISP must have an access token issued by the ASPSP using a client credentials grant." GET is described as retrieving a consent "that they have created"; DELETE is what the AISP does after the PSU has revoked consent with the AISP, not something the PSU drives at the API. So there is no PSU in the session for any of them, by design. The identity that matters is the AISP -- the Consumer the consent was lodged under. All four consent-by-id endpoints -- v3.1 and v4.0.1, GET and DELETE -- were built as if there were. They took the UserOnly ResourceDoc default, they were written with withUser/withUserDelete, which require a user to be present, and their ownership rule, in four verbatim copies, keyed on the caller's user id: Option(consent.userId).forall(_.isBlank) || consent.userId == user.userId That rule is off-spec, not a baseline this change relaxes. It survived only because OAuth2 token parsing hands a client-credentials caller an auto-vivified user that is not a person, so a user id was always there to compare -- the defect described in the previous change in this sequence. The consent's own Consumer, which the copies did check, is the thing the standard actually names. The rule moves to Consent.checkUKConsentAccess, next to validateUKConsentPermissions and for the same reasons: one definition instead of four for a rule whose subtlety is the whole point, and a shape that can be tested without standing up a request. The handlers move to executeAndRespond and executeDelete, and the four ResourceDocs declare UserOrApplication. For a caller with no PSU -- the only caller the standard describes -- access is now decided by the Consumer alone: the AISP that lodged the consent may read and revoke it whatever its status, and no other Consumer may. Previously an authorised consent reached this way was refused with ConsentDoesNotMatchUser. The user check is kept, unchanged, for a caller that does present a PSU. OBP allows credentials the standard does not describe here, and for those the stricter rule still applies: a session acting as one PSU cannot reach another PSU's consent, and the lodging TPP cannot use a PSU session to do it either. That path is a superset of the standard, never a way around it. The rule is unit-tested rather than driven over HTTP because the test framework signs with OAuth1, which always attaches a user: there is no way to make a genuinely PSU-less request from a test. The ResourceDocs get their auth mode pinned separately, since that is what lets such a request reach the handler at all, and nothing else would notice a revert to the default until the token defect is fixed.
#60) In Berlin Group the PSU never calls the API. Under Redirect it authenticates at the ASPSP's own site; under Embedded it hands its factors to the TPP. Everything on the wire is the TPP acting as itself, and the Implementation Guidelines say so for these two in particular (section 4.6, Authorisation Endpoints): the ASPSP "will give access to these sub-resources to the TPP by returning corresponding hyperlinks", and "the authorisation status would still result by submitting the command GET .../authorisations/authorisationId". Both handlers already reflect that. getConsentAuthorisation and getConsentScaStatus use executeAndRespond and never touch cc.user. Only their ResourceDocs disagreed: with no authMode they take the UserOnly default, which sends ResourceDocMiddleware down anonymousAccess, and that returns 401 for a request carrying no user. The consent endpoints in this same file -- POST /consents, GET and DELETE /consents/CONSENTID, GET /consents/CONSENTID/status -- have been UserOrApplication all along, so this is also a consistency fix within the family. No behaviour changes today. A client-credentials caller still resolves to an auto-vivified user, so the UserOnly path never fires; what this does is keep these two working once that stops. Both docs get a scenario pinning the mode, because nothing else would notice a revert until that day. The POST and PUT siblings on the same paths are deliberately left alone. They are the Embedded SCA steps, they carry PSU credentials in the body, and both open with cc.user.openOrThrowException -- so unlike these two they need an answer to "which PSU is this challenge for" before their auth mode means anything. That is a design question, not a contract one.
… REQUESTED (#61) updateStatus guarded its conditional UPDATE on the status it had just loaded instead of the fixed starting status. That only caught the interleaving where two callers read the same old value; when the calls serialise, the second one loads what the first wrote, its guard matches, and it overwrites the decision with no error. A REJECTED application could therefore be re-decided as ACCEPTED — and the ACCEPTED branch of the endpoint opens a bank account, so the overwrite is not recoverable. Guard on REQUESTED, matching the sibling transitions in DoobieBusinessStatusQueries (AccountAccessRequest guards on INITIATED, the challenge CAS on successful_c=false). Reword the zero-row failure, which now also covers "a decision was already recorded", and reuse the same constant for the initial status set at creation. This is what made the M3 race scenario intermittently red: it failed only when the two threads happened to serialise with REJECTED landing first. Add M3b, which reproduces the same defect deterministically with two sequential calls.
…roup authorisation (#62) * fix: resolve the human behind a consent-authenticated consent read GET /obp/v5.1.0/user/current/consents/CONSENT_ID compared the consent's PSU against CallContext.userId, which returns the authenticated principal. Under Consent-Id / Consent-JWT authentication that principal is the per-consent shadow user, not the person, so the comparison could never match and the PSU was told their own consent did not exist. The subject is now CallContext.humanUser, the accessor the codebase already keeps for this distinction and the one checkUKConsent uses for the same comparison. The rule moves into Consent.checkObpConsentUserAccess so it can be stated once, argued once and tested without standing up a request, following validateUKConsentPermissions and checkUKConsentAccess. A consent with no PSU yet stays readable, deliberately: this endpoint is where a PSU inspects a consent before deciding to authorise it, and the app doing the inspecting belongs to the PSU rather than to the TPP that lodged the consent, so the Consumer fallback the standards use would break the journey instead of tightening it. That is where OBP-native's rule parts company with theirs, and why this is its own function. What it leaves open is recorded in the scaladoc: an unbound consent's metadata is readable by any authenticated caller who knows its consent id. Behaviour change: a request authenticated by a consent can now read that consent, where it previously received OBP-35001. Reads by an unrelated user are unaffected and still 404. * fix: bind a Berlin Group consent only for the TPP that lodged it POST /consents/CONSENTID/authorisations and PUT .../AUTHORISATIONID are the two calls that decide who a consent ends up belonging to: the first mints the SCA challenge, the second answers it and writes the PSU onto the consent row. Neither carried an ownership guard, so any authenticated caller could raise a challenge on any consent id and then answer their own, claiming a consent lodged by a different TPP or re-binding one another PSU had already authorised -- updateConsentUser overwrites mUserId unconditionally. Leaving these consents unowned at lodging time, which the standard wants and 6060f42 implemented, widened what that reaches. The Consumer half is the standard's own blanket rule, stated once for the whole API in the Implementation Guidelines, section 4.11 API Access Methods: all methods submitted by a TPP addressing dynamically created resources may only apply to resources created by the same TPP before. A consent and its authorisation sub-resources are such resources, and deleteConsent and getConsentInformation in this same file already enforce exactly that; only the authorisation pair was left without it. The PSU half covers re-binding, which the standard leaves to the ASPSP and says so where it defines PSU-ID: the ASPSP might check whether PSU-ID and token match. That lands on the same rule as checkUKConsentAccess by a different route -- UK's rests on its Endpoints table marking these calls Client Credentials, Berlin Group's on the blanket same-TPP rule plus PSU binding happening at SCA time -- so the two now share one private implementation while each keeps its own argument. Consent.genuinePsu extracts the pseudo-user filter 6060f42 left duplicated inline. It is required here, not tidying: a client-credentials token resolves to an auto-vivified user keyed on the caller's own client key, and comparing that against a consent's real owner would refuse a legitimate TPP poll under the Redirect approach, where the PSU authenticates at the ASPSP rather than through the TPP. Behaviour change: both endpoints now return 403 OBP-35015 when the caller's Consumer did not lodge the consent, and 403 OBP-35023 when a genuine PSU tries to take over a consent already bound to someone else. Both previously succeeded. A TPP that lodges and authorises under one Consumer, which is what the Berlin Group flow describes and what the end-to-end suite exercises, is unaffected. * test: pin what genuinePsu returns when a session carries no PSU Follow-up work on the Berlin Group authorisation handlers builds on this function, and the case it depends on is the one that looks like an absence. Per the standard the caller of those endpoints is the TPP, with the PSU's authentication factors travelling in the request body rather than in the session, so None is the ordinary answer for a conforming call -- not a failure to defend against. Left to a scaladoc, nothing would catch that being narrowed later. Covers the four shapes: no user in the session, only the Consumer's own auto-vivified pseudo-identity, a genuine PSU, and the degenerate case where no Consumer was identified at all. The last one keeps the pseudo-user, since there is no client key to compare against, so it also asserts what checkBerlinGroupConsentAccess then does with it -- refuse on the PSU half when the consent is bound and on the Consumer half when it is not, rather than letting it through. * refactor: share the Berlin Group consent fixtures between both suites The new consent-access suite had its own copy of the consent body, the PSU-less consent builder and the client-credentials session, which the account-information suite already defined. The quality gate caught it as duplicated new code, and it was a maintenance trap besides: the client-credentials fixture encodes a non-obvious fact about how OAuth2 token parsing auto-vivifies a user, and a copy that drifted from it would quietly stop testing the thing it exists for. Moves them into a BerlinGroupConsentFixtures trait that both suites now extend, and drops both copies. No behaviour change -- the fixtures are the account-information suite's originals, moved rather than rewritten.
…aller (#65) POST /consents/CONSENTID/authorisations minted its SCA challenge against cc.user, and the PUT twin bound the consent to that same principal. In Berlin Group that principal is the wrong one. The TPP makes these calls, not the PSU: under Redirect the PSU authenticates at the ASPSP, under Embedded it hands its factors to the TPP, which relays them -- the Implementation Guidelines show it as a TPP request carrying the customer's OTP (V1.3.12, section 6.1.1.4, p.123). What that cost is concrete rather than formal. createChallengeInternal delivers the challenge answer to getEmailsByUserId / getPhoneNumbersByUserId of the user the challenge names, and a client-credentials token resolves to the caller's own auto-vivified pseudo-user. So the OTP was mailed to the TPP and never reached the PSU, and the PUT then wrote that pseudo-user onto the consent, updateConsentUser overwriting mUserId unconditionally. The regression test catches it directly, asserting the challenge's expectedUserId rather than only the consent it produces. Where the standard puts the PSU's identity is the PSU-ID header, which OBP had never read. It is not in the body: psuData carries four password fields and no identifier at all, so an Embedded call cannot name its PSU any other way. Consent.resolveBerlinGroupPsu takes the header, the consent's own PSU and a genuine PSU in the session, and answers in the order the standard's conditionality implies -- PSU-ID is asked for when the ASPSP does not already know (sections 7.1 p.195 and 7.2.1 p.206), so what it already knows wins: 1. the consent's PSU, once SCA has bound one; 2. a genuine PSU in the session, which is the Redirect approach; 3. the PSU-ID header, which is Embedded. None of the three is refused with the code the standard defines for exactly that, PSU_CREDENTIALS_INVALID. A header contradicting 1 or 2 is refused rather than resolved by precedence, which the standard sanctions where it defines the header -- "the ASPSP might check whether PSU-ID and token match" (section 6.3.1, p.134) -- and what it closes is specific: otherwise a lodging TPP could name a third party on a bound consent and have that person's OTP mailed to itself. The PUT no longer needs a session user at all. The challenge already records whose authorisation it is, and getChallenge was being called and discarded one line above, so the consent now binds to the challenge's PSU. That also closes the reverse hole: a client-credentials PUT could previously take a consent off its PSU and onto the caller. Two consequences of reading ownership off the challenge. Its consentId is now checked against the path, because the connector's validateChallengeAnswerC4 matches on challengeId alone and ignores the consentId it is handed -- without it, a challenge minted on one consent could be answered on another's and bind the first consent's PSU to the second. And the OTP is validated as the challenge's PSU rather than as the token's principal, since under Embedded the TPP is only relaying it; the caller's own right to be there was already settled by checkBerlinGroupConsentAccess. Passing a derived CallContext keeps this on the Connector path, so CBS-backed deployments are unaffected. PSU-ID resolves against the local identity provider first, then across providers when exactly one user answers to the username, so a federated PSU still resolves and an ambiguous one is refused rather than guessed. What this deliberately does not do is verify a first factor. psuData.password is still unchecked and the updatePsuAuthentication branch stays mocked, so PSU-ID is an assertion by the TPP. It is the OTP, delivered out of band to the PSU this resolves to, that binds the consent -- which is why resolving it correctly is what makes the unverified assertion safe. All seven ResourceDocs now declare UserOrApplication. b5d556d brought the two GET siblings across and held these back on the grounds that a doc's auth mode says nothing until the handler has an answer to which PSU an authorisation is for. It now has one, and it does not come from the session. Behaviour changes. The OTP goes to the PSU rather than to a client-credentials caller. POST authorisations returns 401 PSU_CREDENTIALS_INVALID where an unclaimed consent has no PSU in the session and no PSU-ID header, having previously minted a challenge for the caller. The consent binds to the challenge's PSU rather than to the session principal. A challenge answered on a different consent's path is refused with 400. A TPP that authorises with the PSU's own token, which is the Redirect journey the end-to-end suite exercises, is unaffected.
…sion principal (#68) GET and DELETE on account-access-consents refused every caller the standard describes. An authorised consent answered 403 OBP-35023 to the AISP polling it with a client-credentials token, and to a request authenticated by the consent itself -- leaving a PSU-signed token, which a TPP never holds, as the only way in. The self-service poll and revoke the endpoints exist for were unreachable. checkUKConsentAccess was not the problem: it already skips the PSU comparison for a caller with no PSU and judges it on the lodging Consumer, and every one of those combinations is unit-tested. The problem is that no caller could produce that input. The four call sites passed cc.user, which is never Empty on a request that reaches these handlers -- a client-credentials token auto-vivifies a pseudo-user keyed on the consumer's own client key, and applyUKRules swaps in the consent's shadow user -- so the rule was asked about the wrong person and the comparison could never match. A well-tested rule kept being handed an identity the tests never covered because no caller could construct one. Consent.actingPsu supplies the missing step: the PSU applyUKRules set aside on consenter if there is one, otherwise whatever genuine PSU the session carries. genuinePsu alone is not enough, because a shadow user's idGivenByProvider is a random UUID rather than the consumer key and so survives that filter -- which is why checkUKConsent already reads consenter at its own PSU comparison, and this follows it. The OBP-native read path had already been through the same fix, and says so at getConsentByConsentId. Consent.assertUKConsentAccess then keeps the rule and the identity it is asked about in one place. The guard was four verbatim copies of the same six lines, and that is how they came to agree on the wrong argument; collapsing them to one call each is what stops the next edit from having to get it right four times. Narrowing is unaffected: a session acting as a different PSU is still refused with ConsentDoesNotMatchUser, and a second TPP with ConsentDoesNotMatchConsumer. Verified against a running instance for both versions and all three credentials: the six calls that returned 403 now return 200, while a different PSU, a different TPP, and DELETE by a different PSU stay refused. Hola's v4.0.1 consent panel, which showed the 403 in place of the consent status, now renders status, permissions and expiry. Both halves of actingPsu are mutation-checked: dropping consenter reds 3 scenarios, dropping the pseudo-user filter reds 2.
#69) POST /obp/v5.1.0/banks/BANK_ID/consents/CONSENT_ID/authorise bound the PSU with updateConsentUser before grantUKConsentAccountAccess had decided whether the request was acceptable, and nothing in the sequence is transactional. A refused attempt therefore claimed the consent for whoever made it: the status stayed AWAITINGAUTHORISATION while mUserId became the caller. That is not a cosmetic leftover. The ConsentDoesNotMatchUser guard at the top of the same endpoint then refuses everyone else, so the genuine PSU can no longer authorise their own consent -- not even start a challenge -- and the lodging TPP loses GET and DELETE on it at the same time, leaving no way back through the API. A consent id is handed to the browser in the authorisation redirect, so it reaches history, referrers and access logs; one failing request from anyone who had seen one was enough to destroy that consent permanently, with no authentication as its intended PSU required. grantUKConsentAccountAccess is the step that rejects an account_id the PSU does not hold, or one that does not exist at this bank, so it now runs first and the three writes follow only once it has passed. Ordering is what has to carry this; there is no transaction to roll back. The reorder is safe for the JWT: grantUKConsentAccountAccess writes the views, and updateConsentUser re-reads the row from the database (MappedConsent.find), so updateUserIdOfBerlinGroupConsentJWT copies the freshly written payload and preserves them. Verified end to end -- after a successful authorisation the consent still returns exactly the selected account. Behaviour change worth noting: a request that is bad in both ways -- wrong OTP and an account the PSU does not hold -- still fails on the OTP, since the SCA check keeps its place ahead of this one. Only the writes moved. Verified against a running instance: a refused authorisation leaves mUserId null and the status AWAITINGAUTHORISATION, and the real PSU can then authorise the same consent normally, which before was permanently impossible. The regression test drives the endpoint over HTTP and asserts the database state rather than the status code -- a 400 was always true, including while the consent was being claimed. Restoring the old order reds it on exactly that assertion.
* fix: check account holdings when a Berlin Group consent binds, and let Redirect SCA reach it
Two problems in the same place, and the first was hiding behind the second.
A Berlin Group consent names its accounts at creation: the TPP lists IBANs in the
access object and createBerlinGroupConsentJWT resolves each to a (bank_id,
account_id) view before any PSU is involved. Nothing then checked that the PSU who
authorises it has anything to do with those accounts. Measured against a running
instance: a consent naming another customer's IBAN, authorised by a PSU who does
not hold it, bound and served that account's details and balances to the TPP.
UK closed this at its own authorise step; Berlin Group never had it.
Consent.assertBerlinGroupConsentAccountsHeld reads the accounts off the consent
JWT -- the same views the read path will materialise, so the two cannot disagree --
and refuses with ConsentAccountNotHeldByUser. It runs at both authorisation steps:
on the POST before the challenge is minted, since an OTP for a consent that can
never bind would only deliver a code to someone the TPP nominated; and again on
the PUT before any write, because the two are separate requests and nothing in
that sequence is transactional.
The second problem is why the first stayed invisible: the Consumer half of
checkBerlinGroupConsentAccess refused Redirect SCA outright. The standard's
same-TPP rule binds "methods submitted by a TPP" (Implementation Guidelines 4.11),
but under Redirect the PSU authenticates at the ASPSP and the call arrives from
the ASPSP's own front end -- not a TPP, and never the Consumer that lodged the
consent. The scaRedirect ceremony could not complete at all.
Nothing in the request separates that front end from a second TPP holding a PSU
session, so the ASPSP declares its own: berlin_group_sca_front_end_consumer_ids,
empty by default, which leaves the same-TPP rule applying to every caller. A
declared front end skips that half only; a consent already bound to a PSU still
re-binds to that PSU alone.
The Consumer check was never what protected the consent -- the TPP that lodged it
passes by definition, and is the party the access accrues to. That is what the
holdings check is for, and why the two changes belong together.
Verified end to end through the Portal: the scaRedirect ceremony now completes
(POST authorisations 201, PUT 200, consent valid and owned by the real PSU) and
account, balance and transaction reads all return. Still refused: a consent naming
an IBAN the claiming PSU does not hold (403, and the consent is left untouched),
and an authorisation started by a Consumer that neither lodged the consent nor is
a declared front end (403).
* fix: report a refused UK consent authorisation as itself, not as a connector fault
grantUKConsentAccountAccess returns a Failure carrying its own reason -- an
account_id the PSU does not hold, or one that does not exist at this bank. Passing
that Box through connectorEmptyResponse rewrote every one of them into
InvalidConnectorResponse at 400, so what reached the TPP was
OBP-50200: Connector cannot return the data we requested. connectorEmptyResponse
<- OBP-35037: One or more of the specified account_ids is not held by ...
An authorisation decision presented as a connector fault, with the actual reason
trailing behind a cause it has nothing to do with, and a status code that says the
request was malformed rather than refused.
A Failure here is the same kind of answer as the ConsentDoesNotMatchUser guard a
few lines above, so it now gets the same treatment: its own message, at 403. Only
a genuinely empty Box is still treated as a connector problem, which is what
connectorEmptyResponse is for.
The existing regression test asserted `code should not equal 200`, which was true
of the wrapped 400 as well; it now pins 403 and that neither OBP-50200 nor
connectorEmptyResponse appears in the message. Mutation-checked: restoring
connectorEmptyResponse reds it on the status assertion.
Regression: code.api.UKOpenBanking 394/394, code.api.v5_1_0 245/245,
code.api.berlin.group 180/180.
* fix: refuse a non-UK consent at a UK endpoint instead of throwing
Presenting an OBP-native consent to a UK Open Banking endpoint came back as
500 OBP-50000: Unknown Error.: Not found http request header 'Authorization',
it is mandatory.
A server fault for a request that was merely not entitled, and the Berlin Group
side already answers the mirror case cleanly with OBP-35036.
The shape of such a request explains the throw. The auth dispatcher routes the
consent into its own standard's branch, that branch authenticates the request, and
ukConsentId is left unset because applyUKRules never ran. checkUKConsent then finds
no Authorization header to read a consent_id claim from, and threw rather than
returning the Box it is declared to return.
It now returns Failure(ConsentDoesNotMatchStandard), which is the same refusal the
Berlin Group path gives for a UK consent, so the two standards answer their mirror
cases the same way. Verified against a running instance: 500 becomes 403 OBP-35036,
a UK consent at the same endpoint still returns 200, and a request carrying nothing
at all is still 401.
The short-circuit for consent-header authentication is untouched and now has a test
of its own, since that is the path the refusal must not swallow.
Mutation-checked: restoring the throw reds the new scenario with the
RuntimeException itself. Regression: code.api.UKOpenBanking 396/396,
code.api.berlin.group 180/180, code.api.v5_1_0 245/245.
* fix: keep the internal principal out of Berlin Group view refusals
A refused Berlin Group read answered with
OBP-20060: User does not have access to the view: ReadBalancesBerlinGroup
userId : 8989636c-3879-4f6d-86cc-e79e8d44e388. account : 56fb36df-...
Under consent authentication that user id is the consent's own shadow user: an
internal identifier minted per consent, which the TPP was never party to and cannot
act on. The refusal is about a view and an account, both of which the caller named
itself, so the message now says only that. The user id is still logged for anyone
diagnosing the refusal.
Mutation-checked: putting the user id back reds the new scenario.
Regression: code.api.berlin.group 180/180 plus the new scenario,
code.api.UKOpenBanking 396/396.
* fix: a Berlin Group payment is only addressable by the party that initiated it
Berlin Group names a payment by its id alone -- /{paymentService}/{paymentProduct}/
{paymentId} carries no account -- so nothing in the route tied a payment to its caller.
Every payment-scoped route fetched it by id and went ahead. A paymentId was therefore a
bearer token: any authenticated TPP holding one could read the payment and its status,
list its authorisations, start an authorisation on it, and cancel it. Starting an
authorisation is the serious one: the challenge is minted for the caller's own user id,
so the second TPP could then answer it and execute someone else's payment.
Under NextGenPSD2 a payment initiation resource belongs to the TPP that created it, and
only that TPP addresses it afterwards. The initiating identity was already being recorded
on the payment (user_id, plus on_behalf_of_user_id when it was lodged under a consent);
nothing read it back. Fetching now goes through getOwnPaymentImpl, which compares those
two against the two the caller presents -- its principal and, under consent
authentication, the PSU it is acting for. Any overlap is enough, so a payment lodged on a
client-credentials token can still be authorised under the PSU's token and the other way
round. A payment carrying neither identity belongs to nobody and is refused.
All eleven payment-scoped routes go through it, including GET
/{paymentId}/cancellation-authorisations, which previously listed a payment's
cancellation authorisation ids without fetching the payment at all. That endpoint now
answers a non-existent paymentId the way its ten siblings already did, rather than with
an empty list; its scenario is updated to match.
Mutation-checked: making the guard always pass reds the new scenario on the first refusal
it asserts. Regression: code.api.berlin.group 182/182.
* fix: report the direction of a UK transaction instead of always saying Credit
UK Open Banking splits a signed amount in two. Amount is unsigned -- the pattern for
OBActiveCurrencyAndAmount_SimpleType is ^\d{1,13}$|^\d{1,13}\.\d{1,5}$, which no negative
string matches -- and the direction sits beside it in CreditDebitIndicator
(OBCreditDebitCode: Credit | Debit). OBP holds the same fact the other way round, as one
signed BigDecimal.
Neither half was being done. Every factory passed the signed number straight into Amount
and hardcoded "Credit" next to it, so a debit of 25 was reported as a credit of -25:
wrong in both fields at once, and non-conformant in Amount whatever the direction.
A TPP reading the account could not tell money in from money out.
UKAmounts does the split once, shared by the v2.0, v3.1 and v4.0.1 factories rather than
copied into each, and every transaction and balance now goes through it. Zero is a credit,
which the standard states explicitly. A balance OBP holds as a string that will not parse
is passed through untouched rather than turned into a fabricated zero.
The "Credit" defaults are gone from the two case classes as well: a default is how the
literal reached every debit in the first place, so the direction now has to be supplied
at each construction site.
This is the CreditDebitIndicator half of UK-1. The other half -- ReadTransactionsCredits
and ReadTransactionsDebits not filtering the returned list -- is the follow-up already
noted at constant.scala:685-694, and depended on this.
Mutation-checked: restoring the literal and the signed amount reds 4 of the 7 new
scenarios. Regression: code.api.UKOpenBanking 403/403.
* fix: count Berlin Group accesses that carry no PSU against frequencyPerDay
frequencyPerDay is "the requested maximum frequency for an access without PSU involvement
per day", so everything turns on how the ASPSP decides no PSU was involved. NextGenPSD2
settles that with one header: on every AIS read and consent-management call,
PSU-IP-Address "shall be contained if and only if this request was actively initiated by
the PSU" (parameter PSU-IP-Address_conditionalForAis).
isTppRequestsWithoutPsuInvolvement read it the other way round. Only a request carrying
PSU-IP-Address: 0.0.0.0 or a no-psu-involved device header counted; a request that simply
omitted PSU-IP-Address -- the exact shape the standard reserves for unattended access --
was never counted at all. Since getHeaderValue answers a random long for a header that is
absent, absence could not match anything by construction. Each TPP therefore decided
whether its own daily limit applied to it, by opting in or not.
Absence of PSU-IP-Address is now the declaration it is defined to be. The two sentinels
are still honoured, for a TPP that sends the header unconditionally and marks the
unattended case in its value instead. Header lookup is case-insensitive, as HTTP requires,
and a blank value counts as absent.
Mutation-checked: restoring the sentinel-only reading reds the two scenarios about an
absent header. Regression: code.api.berlin.group 182/182,
code.api.UKOpenBanking 403/403.
* fix: spend one frequencyPerDay access per request, not one per middleware
A Berlin Group consent asking for four accesses a day got none: the first unattended call
answered 429, with usesSoFarTodayCounter already stamped to 4. Measured across several
limits, one HTTP request always spent the whole allowance --
frequencyPerDay=2: one request -> 429, counter 0 -> 2
frequencyPerDay=3: one request -> 429, counter 0 -> 3
frequencyPerDay=6: one request -> 429, counter 0 -> 6
-- which is not what checkFrequencyPerDay does. It grants exactly frequencyPerDay
accesses. It was simply being asked many times per request.
The authentication pipeline runs once per API version in the route chain: each version
wraps its own routes in its own ResourceDocMiddleware, and a middleware whose index holds
no matching doc still runs best-effort authentication before falling through to the next
one (ResourceDocMiddleware's `case None` branch). Roughly ten passes per request, each
carrying the Consent-ID header into applyBerlinGroupRules and spending an access. The
comment above the call -- "This function MUST be called only once per call" -- states a
precondition its caller has never met.
Only the middleware that matched a ResourceDoc attaches it to the CallContext, so its
presence identifies the pass that will actually serve the request. Both the check and the
increment are now gated on that, which is also what "an access" means.
The wider consequence of those extra passes -- the whole pipeline, its consent
validation, signature verification and database reads, running ~10x per request for every
endpoint -- is left alone here: the fallthrough is deliberate, so that is a decision to
take on its own rather than a side effect of this fix.
Measured against a running server, frequencyPerDay=4: four 200s, then 429, counter
following 1,2,3,4. A request carrying the PSU's address is still not counted at all.
Regression: code.api.berlin.group 182/182.
* fix: hold a Berlin Group payment to the TPP that lodged it, not only to the PSU
The ownership guard added in the previous commit compared the people a payment records
against the people the caller presents. That leaves the case Berlin Group actually cares
about: a payment initiation belongs to the TPP that created it, and two TPPs can serve
the same PSU. A second TPP calling with the same PSU's credentials matched on the person
and was let straight through -- GET /{paymentId}/status answered 200 under a different
consumer key.
The scripted probe caught it; nothing in the repository could have, because the payment
recorded no consumer to compare against. It records one now, alongside the user ids that
were already there, and addressing a payment requires the TPP to match as well as the
person. Payments lodged before the column existed carry no consumer and fall back to the
person check rather than becoming unaddressable.
The new scenario needed a caller shape DefaultUsers does not have -- user2 and user3
change the person as well as the consumer -- so it issues resourceUser1 a token under
testConsumer2: same person, different TPP.
Mutation-checked: dropping the TPP comparison reds that scenario and only that scenario.
Regression: code.api.berlin.group 183/183.
* fix: release a VRP mandate's view and limit when its consent is revoked
Converting a VRP consent-request builds a private custom view named _vrp-<uuid>, grants
it to the PSU, hangs a counterparty off it and gives that counterparty a limit. Together
they are the mandate: the view carries CAN_ADD_TRANSACTION_REQUEST_TO_BENEFICIARY, and
the limit is how much may be paid under it.
Revoking the consent dropped only the shadow user's access. The PSU kept a live standing
payment authority for a mandate they had just cancelled, and a set of these accumulated
on the account for every mandate ever requested -- one test account had collected three
_vrp- views, two of them from consents that were never even used, and 52 limit rows.
Nothing in the API removed any of it.
Each artefact is named after the view and the view belongs to exactly one consent, so
this can be undone without guessing. Revocation now gives back the PSU's grant, deletes
the counterparty's limit, and removes the view -- but only once no access row still points
at it, which removeCustomView already refuses to do otherwise. So a view something else
still holds is left in place rather than orphaned.
The counterparty row stays. It is a payee record that settled transactions refer to, and
deleting it would take history with it; with the view and the limit gone it grants
nothing.
The release runs outside the shadow-user lookup and after it. Outside, because a VRP
consent that never reached SCA has no shadow user and its mandate still has to be
released -- an abandoned mandate is exactly the case that accumulated. After, because the
view can only go once every access row is gone, the shadow user's included. Getting this
wrong is what the new scenario caught: placed inside the comprehension, it never ran at
all for an unauthorised consent.
The "_vrp-" prefix now lives in Constant, read by both the conversion that writes it and
the revocation that looks for it.
Regression: code.api.v5_1_0.VRPConsentRequestTest 7/7 including the new scenario. Both
harness probes that measured this now pass against a running server.
* fix: give UK v2.0 and v3.1 amounts the member names the standard specifies
UK Open Banking writes an amount as {"Amount": "...", "Currency": "..."} --
OBActiveOrHistoricCurrencyAndAmount, both members capitalised. The v2.0 and v3.1
factories emitted {"currency": ..., "amount": ...}, because they reused OBP's shared
AmountOfMoneyJsonV121, which spells the same two members in lower case. Every amount in
both versions was affected: transaction amounts, charges, instructed amounts, balances
and credit lines. v4.0.1 already had its own AmountV401 and was correct.
The shared class is used by OBP's own endpoints and cannot be renamed, so the UK
responses take their own shape, as v4.0.1 already does.
This one was hiding a second defect. A probe that read only the lower-case spelling saw
no debit in a v4.0.1 response, concluded there was none to check, and passed -- so the
CreditDebitIndicator bug fixed in 944b592 stayed green in the harness until the casing
was noticed.
v2.0's balances also reported the account owner's *name* as the CreditDebitIndicator, in
a field the standard restricts to Credit or Debit. It is derived from the balance now,
like every other amount here. The Type field on those same balances says "Credit", which
is not a member of OBBalanceType1Code either; that one needs a decision about which
balance type is meant, so it is left alone and recorded rather than guessed at.
Regression: code.api.UKOpenBanking 403/403, code.api.ResourceDocs1_4_0 90/90,
code.api.v5_1_0 246/246. No test asserted the lower-case spelling.
* fix: resolve an account by a registered OBP routing, not only by the implicit one
The OBP account-routing scheme means two things at once. It is an implicit self-identifier
-- an address under it is normally the account id, with no row in bankaccountrouting --
but a bank may also register an OBP routing whose address is something else entirely, and
that row is stored like any other scheme's.
getBankAccountByRoutingLegacy honoured only the implicit reading, so an account with a
registered OBP routing was unreachable through every endpoint that resolves by routing.
The row was right there in the table and the answer was "Bank Account not found", which
is what stopped a consent-request naming an account that way from ever converting:
{"scheme":"OBP","address":"hola-testuser01-uk-current"} -> 404 OBP-30073
{"scheme":"OBP","address":"726b08a5-..."} (the account id) -> 201
{"scheme":"IBAN","address":"DE89..."} -> 201
The implicit reading is tried first and still wins wherever both would match, so no
address that resolves today resolves differently. The fallback runs only when the implicit
reading finds *nothing*.
Not when it finds an ambiguity. The first version of this used `or`, which also replaced
"this address matches several accounts" with whatever the routing table said -- nothing --
turning a precise complaint into a bare "not found". The new scenario caught that; reading
the diff would not have.
Mutation-checked: removing the fallback reds the registered-routing scenario alone.
* fix: the same OBP-routing blind spot in the plural resolver, which VRP uses
Found by filling in the VRP form in a browser rather than by the API probes. Naming the
debtor account by its registered OBP routing address failed at consent-request creation,
one step earlier than the conversion fixed in the previous commit and in a different
function:
404 OBP-30018: Bank Account not found. Please specify valid values for BANK_ID and
ACCOUNT_ID. Current BankId is gh.29.uk.x1 and Current AccountId is
hola-testuser01-uk-current
getBankAccountByRoutings -- the plural one, which createVRPConsentRequest calls -- carries
its own copy of the implicit-OBP shortcut and had the same blind spot as the singular
resolver. Two copies of one rule, so fixing the first did not fix the second.
It now asks the resolver that knows both readings, and still falls back to
checkBankAccountExists when neither answers, so a genuinely unknown account reports itself
exactly as it did before.
Verified end to end afterwards: the VRP consent-request converts, the mandate binds, and
revoking it releases the view, the PSU's grant and the limit while leaving the PSU's own
ten baseline access rows untouched.
Regression: the routing suite 5/5, including a new scenario for the plural resolver.
* fix: keep the lodging TPP off the connector wire contract
RestConnector_vMar2019_FrozenTest went red, and it was right to. Adding consumer_id to
TransactionRequest changed the frozen structure of a type the REST connector sends and
receives, so every connector implementor would have seen a new field appear -- for a fact
only one server-side guard needs.
The field is reverted from obp-commons and from toTransactionRequest. The mConsumerId
column stays, because that is where "which TPP lodged this payment" belongs, and the
Berlin Group ownership guard reads it straight off the stored row instead.
Same behaviour, no change to the connector contract: the frozen test passes again and the
Berlin Group suite still holds, including the scenario where a second TPP acting for the
same PSU is refused.
Regression: code.connector.RestConnector_vMar2019_FrozenTest 5/5,
code.api.berlin.group 183/183.
* fix: let the ASPSP's own approval screen read a UK consent nobody has claimed yet
The UK approval screen showed the PSU a bank and a consent id and nothing else -- no
permissions, no status, no expiry -- so they were asked to approve a consent without
being told what it granted. The markup was there all along; the data never arrived.
The screen fetches the consent to fill those fields, and it arrives under its own
Consumer rather than the TPP's. The lodging-Consumer comparison therefore refuses
precisely the caller whose job is to inform the PSU: 403 OBP-35015. The loader treats that
as non-fatal and renders the page bare, which is why it looked like a display bug.
This is the same difficulty the Berlin Group Redirect flow already hit, and it takes the
same answer: nothing in a request distinguishes the ASPSP's own screen from a second TPP
holding a PSU session, so the ASPSP declares which Consumer is its own. The props key is
generalised to sca_front_end_consumer_ids, since it was never Berlin-Group-specific; the
old berlin_group_sca_front_end_consumer_ids is still read, so a configured instance needs
no edit.
Narrow on purpose. It applies only while the consent is unclaimed -- the window the
approval screen exists for -- so once a PSU is bound, the PSU comparison governs and a
declared front end gets no further than anyone else. It is inert unless an ASPSP declares
a front end at all, which is the default.
Measured against a running server: the screen now shows AWAITINGAUTHORISATION, the expiry
and all four requested permissions, while an undeclared Consumer reading the same
unclaimed consent is still refused 403.
Mutation-covered by three new scenarios: a declared front end may read an unclaimed
consent, may not read a claimed one, and an undeclared caller is still refused.
Regression: code.api.UKOpenBanking 406/406, code.api.berlin.group 183/183.
* fix: make ReadTransactionsCredits and ReadTransactionsDebits actually restrict the rows
These are independently-selectable Permissions in the UK profile, and the ASPSP must refuse a
consent that names a transactions depth without at least one of them -- which OBP already
does. Then it returned every transaction regardless. A consent granting Credits only still
returned the debits, so the PSU's choice of direction was decorative and the TPP saw money
going out of an account it had only been permitted to watch coming in.
This is the follow-up recorded at constant.scala:685-699. It was blocked on
CreditDebitIndicator being a hardcoded literal, since a filter and a label that disagree are
worse than neither; that was fixed in 944b592, so the direction is now derivable and both
read it from the same place.
Applied in the endpoint rather than as a can_* permission, as that note decided: direction
restricts which rows come back, not which fields are visible, so the view's permission set is
the wrong instrument. The two direction views are resolved exactly as Detail-or-Basic already
is, and holding both -- or neither -- restricts nothing: neither is the plain Basic case, both
is a TPP asking for everything.
Shared by v3.1 and v4.0.1 rather than written twice, which is also what keeps the filter and
the label from drifting apart.
Mutation-checked against a running server: disabling the filter reds exactly the four
direction-restricted cases across both versions and leaves the two unrestricted ones green.
Repro: .local-testing/OBP-Hola/uk_direction.py, 6/6.
Regression: code.api.UKOpenBanking 410/410, code.api.berlin.group 183/183.
* fix: address the review findings on this branch
Five fixes to my own earlier commits, found by reviewing the branch as a whole.
**Direction restriction was applied after the page limit.** The filter trimmed a page the
database had already limited, so a direction-restricted consent got a short page it could
not tell from the end of the data -- and with Constant.Pagination.limit defaulting to 50,
with no pagination parameter from the TPP at all. On an account whose first page is
debits, a Credits-only consent saw one row where eleven existed. The restriction is now
pushed into the query as OBPTransactionDirection so the database applies it and the limit
together; the endpoint filter stays, because a connector other than the mapped one may
ignore the param and that filter is what actually enforces the consent's scope.
The two-transaction fixture is why the earlier tests could not catch this. The new probe
seeds sixty debits ahead of ten credits, and mutation-checking it against the previous
behaviour reds all four cases.
**A declared SCA front end could reach a consent that already had a PSU.** The Berlin
Group guard fell through to the front-end exception whenever the caller presented no PSU,
which is exactly what a client-credentials caller presents -- so the exception applied to
consents already bound to somebody else, the opposite of what its own paragraph promises.
Now conditioned on the consent being unclaimed, as the UK twin already was.
**The counterparty-limit deletion was fire-and-forget.** Its Future was discarded and
getCounterparties' Box was flattened with getOrElse(Nil), so a failed lookup or a failed
delete left the standing limit alive -- the very leak the commit fixes -- while the code
logged "released". Both are observed now, and say so when they fail.
**unsignedAmount used toString.** BigDecimal renders a negative scale in scientific
notation, so BigDecimal("1E+3") came out "1E+3", which the Amount pattern this exists to
satisfy rejects. toPlainString.
**A superseded scaladoc block** was left stacked above scaFrontEndConsumerIds, still
asserting the key is Berlin-Group-specific.
New scenarios cover the plain rendering, the query restriction, and that the restriction
and the post-filter agree on every amount -- two enforcements of one rule that must not
diverge.
Regression: code.api.UKOpenBanking 413/413, code.api.berlin.group 183/183,
code.api.v5_1_0 246/246, RestConnector_vMar2019_FrozenTest 5/5.
* fix: address the second review pass and the duplication gate
A cleanup failure must not fail a revoke that already happened. The Await
added to observe the counterparty-limit deletion runs after the status flip
has committed and after the shadow user's access is gone, and a second
attempt is refused with ConsentAlreadyRevoked -- so letting a timeout or a
connector failure escape turned a completed revoke into a 500 and abandoned
the views still queued behind it. Caught and logged instead.
A transaction whose amount the view withheld is now admitted by neither
direction. The filter reads already-moderated rows, so a missing amount means
the view did not grant CAN_SEE_TRANSACTION_AMOUNT rather than that the amount
is zero; creditDebitIndicator maps that to Credit for labelling, which as a
permission test handed every debit to a Credits-only consent.
The direction boundary now lives once, on OBPTransactionDirection, and both
enforcements build from it: the connector's SQL predicate and the endpoint
filter the test checks against. The scenario claiming the two agree used to
model the query in the test itself, so it agreed with its own copy and could
not detect the drift it existed to catch.
The v3.1 and v4.0.1 transaction reads were the same thirty lines with a
different factory at the end, which is why the direction rule had to be
written into both. They now share UKTransactionsQuery and keep only their
route and their yield. LocalMappedConnector's two identical query builders
are likewise one method. That also clears the duplication gate the previous
commit pushed over its threshold.
Remote connectors take a frozen outbound message that cannot carry the
direction, so they still return both and the filter trims an already-limited
page. Nothing here can repair that without the connector, but returning the
short page silently is how the defect stayed invisible: a full page that lost
rows to the filter is now logged with the connector that needs the param.
* fix: refuse listing the authorisations of another TPP's consent
GET /consents/CONSENTID/authorisations had passesPsd2Aisp and nothing else,
so any AISP caller could list the challenge ids of a consent lodged by
somebody else. Its immediate neighbour, GET /consents/CONSENTID, already
compares the consent's consumer against the caller's and answers 403 -- two
reads of the same consent, twelve lines apart, disagreeing about who may
perform them.
The PUT that answers a challenge is guarded, so this leaked identifiers
rather than access. Guarded the same way as the sibling.
* fix: stop answering unrecognised authorisation requests with a canned example
Berlin Group hangs four request bodies off one authorisation path, and the
handlers dispatch on the body's shape. Only transactionAuthorisation was ever
recognised, and the fall-through was a hardcoded example rather than an
error -- so anything else got a fabricated success.
Two consequences, both reachable by a conforming TPP:
An empty body is how the standard starts an authorisation, and it fell to
that fall-through. The caller got 201 with the literal authorisationId
"123auth456.", then discovered at the PUT that the id matched no challenge.
Neither start handler reads scaAuthenticationData -- the POST mints the
challenge and the PUT answers it -- so an empty body and a
transactionAuthorisation body are the same request here, and both now start a
real authorisation.
On the PUT, the final else was labelled "authorisationConfirmation variant"
but tested nothing, so an unreadable body was answered "scaStatus":
"finalised" -- the terminal success state of strong customer authentication --
for an authorisation nothing had happened to. It is guarded by the checker
that already existed for it.
What remains mocked is what is declared as mocked: the updatePsuAuthentication
and selectPsuAuthenticationMethod Embedded steps. Everything else is now a
400 that says which shapes are accepted.
None of this was reachable through OBP's own clients, which always send
{"scaAuthenticationData": ""} -- the single branch that worked was the only
one ever exercised.
) Four endpoints read a Berlin Group consent by id. Two compared the consent's lodging Consumer against the caller's; two did not, and answered any AISP that knew the consent id: GET /consents/{id}/status -> consentStatus GET /consents/{id}/authorisations/{id} -> scaStatus Both were already fetching the consent, to prove it exists, and then simply did not look at who was asking. So the id alone confirmed a consent existed and let its progress through authorisation be watched from outside. The PUT that answers an authorisation is guarded, so this disclosed state rather than granting access. Guarded the same way as the two that were already right. Verified with two TPPs against a running instance: the stranger now gets 403 on all four while the lodging TPP still gets 200 on all four. The probe covering this was the direct cause of the gap -- it asserted the one endpoint that had been noticed, so the other three went unexamined. It now walks the whole family, which is why the two that were open showed up at all.
oss.sonatype.org was retired and now answers 403 to everything, so the two repositories pointing at it could only ever fail. They did so intermittently rather than always, which is what made this hard to attribute: Maven consults a remote repository only when a POM is not already in the local cache, so a warm runner never touched them while a cold one failed the whole build before a single test ran. The same commit passed in one CI run and failed in the next, on a shard unrelated to anything that had changed. Nothing was ever resolved from them. Verified by resolving the full reactor, and then compiling it, against an empty local repository with both removed: BUILD SUCCESS, 2622 artifacts from Central and 12 from JitPack, zero requests to the dead host and zero unresolved artifacts or plugins. pluginRepositories contained only the dead entry, so the block goes with it; Maven consults Central for plugins by default, which the cold compile exercised. The two repositories that remain are load-bearing: git-OpenBankProject serves OBP's own published artifacts and jitpack.io serves the pinned lift-persistence build.
…ts (#73) grantAccessToViews took a consumerId that no caller passed. All four call sites use the default, ALL_CONSUMERS, and the two methods the branch reached for -- grantAccessToSystemViewForConsumer and grantAccessToCustomViewForConsumer -- have no other caller anywhere in the codebase. Dead, but not harmless. AccountAccess.consumer_id stores the literal string ALL_CONSUMERS rather than acting as a wildcard, and every lookup matches it by equality. revokeConsentAccountAccess sweeps a revoked consent's rows by asking for exactly ALL_CONSUMERS, so any row written under a real consumer id would have been invisible to it: access outliving the consent that created it, with nothing left to remove it. Whoever first passed a consumerId would have got that silently, and the two functions are far enough apart that the connection is easy to miss. Removing the branch makes the invariant hold by construction rather than by coincidence, and both sides now say so: grantAccessToViews carries the warning, revokeConsentAccountAccess records that its sweep is complete only because of it. The scaladoc also claimed UK grants to the real PSU and passes the consent's own consumerId, which stopped being true when UK moved to the shadow user. The Views trait keeps both ForConsumer methods; only this caller goes. berlin.group 183/183, UKOpenBanking 414/414, v5_1_0 246/246. The local probe matrix gains a check that reads the granted rows back and asserts every one is scoped ALL_CONSUMERS, so the property revoke depends on is pinned rather than assumed: 103/103.
…as the PSU (#74) * refactor: remove the unused consumer-scoping branch from consent grants grantAccessToViews took a consumerId that no caller passed. All four call sites use the default, ALL_CONSUMERS, and the two methods the branch reached for -- grantAccessToSystemViewForConsumer and grantAccessToCustomViewForConsumer -- have no other caller anywhere in the codebase. Dead, but not harmless. AccountAccess.consumer_id stores the literal string ALL_CONSUMERS rather than acting as a wildcard, and every lookup matches it by equality. revokeConsentAccountAccess sweeps a revoked consent's rows by asking for exactly ALL_CONSUMERS, so any row written under a real consumer id would have been invisible to it: access outliving the consent that created it, with nothing left to remove it. Whoever first passed a consumerId would have got that silently, and the two functions are far enough apart that the connection is easy to miss. Removing the branch makes the invariant hold by construction rather than by coincidence, and both sides now say so: grantAccessToViews carries the warning, revokeConsentAccountAccess records that its sweep is complete only because of it. The scaladoc also claimed UK grants to the real PSU and passes the consent's own consumerId, which stopped being true when UK moved to the shadow user. The Views trait keeps both ForConsumer methods; only this caller goes. berlin.group 183/183, UKOpenBanking 414/414, v5_1_0 246/246. The local probe matrix gains a check that reads the granted rows back and asserts every one is scoped ALL_CONSUMERS, so the property revoke depends on is pinned rather than assumed: 103/103. * fix: refuse a UK consent that names no account instead of serving it as the PSU A consent authorised before grantUKConsentAccountAccess existed still carries the (null, null, permission) placeholder views createUKConsentJWT writes at creation time. resolveUKConsentPrincipal fell back to running those as the PSU, which is the widest possible reading of a consent that selected nothing: the PSU's own AccountAccess rows govern, so the TPP saw everything the PSU can see and the consent's declared Permissions constrained none of it. The scaladoc said so plainly and it was still the behaviour. "We cannot tell which accounts this consent covers" is a reason to serve nothing, not a reason to serve everything. Refused now, with OBP-35040 telling the caller to re-authorise -- which binds the consent to accounts and makes it work again. Nothing created today can reach this. The authorise endpoint rejects an empty account_ids with 400, so every consent it accepts names real accounts; these are rows from older versions and nothing else. The local database has four authorised UK consents and all four are bound, so the branch is unreachable here -- which is exactly why it needed a test that builds one deliberately rather than waiting to meet one. uk_consent_allow_unbound_legacy restores the old behaviour for an operator who needs a migration window and accepts what it means. Default false, and it warns on every use. Also corrects the pom comment from the previous change: oss.sonatype.org does not answer 403 to everything, it 302-redirects to Central and answers 403 to GitHub Actions runners. The conclusion is unchanged and stronger -- a pure redirect to Central serves nothing Central does not -- but the stated reason was wrong, and the comment now records the _remote.repositories consequence for existing local caches. UKOpenBanking 414/414, berlin.group 183/183, v5_1_0 246/246. The probe matrix rebuilds a genuine unbound consent by re-signing its JWT with the consent's own secret and drives it over HTTP: 105/105. Mutation-checked by enabling the escape hatch in a real build -- exactly the two new checks fail, and they fail with 200 and a body, which is the old behaviour they exist to catch.
grantAccessToViews reconciles a consent's shadow user against the views the consent declares: it grants what is missing and revokes what is no longer declared. The grant half collects its results and fails the request when one of them fails. The revoke half discarded its results entirely, so a revoke that failed left the shadow user holding a view the consent had given up, with no trace anywhere. canRevokeOwnerAccess is the realistic way to reach that: it refuses to drop an `owner` row when no other principal holds one on the account, and an OBP-native consent can carry `owner` because createConsentJWT takes its views from whatever the PSU already holds. Narrowing such a consent then silently keeps the wider access. Log rather than fail. Returning a Failure here would make a consent stuck in this state unusable altogether, including the views it still legitimately holds, and leave the operator no way back short of editing rows by hand. The access it should have lost is a smaller problem than the access it should have kept, so the request is served and the discrepancy is recorded at warn with the consent, user, account and view named.
…m, and document both cases (#76) * fix: report the revokes the consent revocation sweep could not perform revokeConsentAccountAccess counted its revoke attempts, not its successes: .map { access => revokeAccessToViewForUserAndConsumer(...) }.size so "dropped N account access rows" named rows that were still in the table. revokeAccessToViewForUserAndConsumer applies the same canRevokeOwnerAccess rule as revokeAccess, and will not drop the last `owner` row on an account, so the refusal is reachable here too. This is the worse place for it to happen. When grantAccessToViews cannot revoke a stale view, the consent is live and revoking it would still clean up. Here the consent is already revoked and the access it created outlives it, with nothing left in the system that will ever come back for that row -- while the log asserted it had been removed. Count only the successes, and warn once per row that stayed, naming the account, the view and the consent so it can be cleared by hand. Introduced in 01826ce. * docs: runbook for consent access that could not be revoked Both warnings mean access is still held that a consent no longer covers, and neither self-heals. The runbook says what the line means, why the request is still served, how to tell the two cases apart, and what to do. The diagnosis leans on one point that is easy to miss: for a shadow user, a refusal means no other principal holds `owner` on that account -- not even the PSU. That is a finding about the account, not just the consent, and restoring the PSU's own access lets the next use of the consent revoke the stale row by itself, which is the only resolution that does not involve editing rows by hand.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Everything on
develop-obpsince #2868. 69 commits, 75 files, +6101/-589 — 45 main, 21 test, 3 docs.The bulk of it is one theme: a consent's access must be resolved against the PSU who granted it, and against the TPP the consent was lodged for. Several endpoints resolved it against whoever happened to be authenticated on the call instead, which is a different principal in every flow that matters.
Consent identity and ownership
/consents/{id}/statusand/authorisations/{aid}fetched the consent to prove it existed but never asked who was asking — any TPP with valid AISP credentials could read another TPP's consent status and watch it move through SCA. The other two endpoints in the same family already checked.UK consent lifecycle
OBP-35040telling the PSU to re-authorise;uk_consent_allow_unbound_legacy(defaultfalse) keeps a migration window. The authorise endpoint already rejects an emptyaccount_ids, so this shape cannot be created today.grantAccessToViews.AccountAccess.consumer_idstores the literalALL_CONSUMERSand every lookup is an equality match, while the revoke sweep only ever asks forALL_CONSUMERS— so a row written under a real consumer id would have been invisible to revocation forever. Dead, but not harmless: deleting it makes that invariant structural instead of incidental.grantAccessToViewsdiscarded the result of revoking stale access, and the revocation sweep counted attempts as successes, sodropped N account access rowsnamed rows that were still present.canRevokeOwnerAccessrefuses to drop the lastownerrow on an account, which makes both reachable. Now counted and logged.docs/operations/CONSENT_STALE_ACCESS.mdis the runbook.Correctness and infrastructure
UPDATE ... WHERE mstatus='REQUESTED'.Tests
21 test files, ~+2400 lines, mostly new suites rather than adjusted assertions:
UKOpenBankingV401ConsentScopingTests,UKOpenBankingV401ConsentAccessTests,UKOpenBankingV401ConsentPermissionsTests,UKOpenBankingV401AccountInfoTestsBerlinGroupV13ConsentAccessTests,BerlinGroupConsentFixturesUKAmountsTest, plus additions to the existing v3.1.0 and BG AIS/PIS suitesEach ownership fix is asserted from two callers — the consent's own TPP and a stranger TPP — because a single-caller assertion is exactly what let #71 sit behind an earlier fix to the same endpoint family.
Verification beyond CI
Driven end to end against a local stack (OBP-API, OBP-OIDC, Portal, OBP-Hola) across five flows — UK v3.1, UK v4.0.1, Berlin Group, OBP-native, VRP — with a scripted suite of 105 checks, all green on
49723b421, plus manual browser runs of each flow. The probes assert against the database rather than status codes where it matters: a 4xx can be truthful while state is still being mutated behind it.Tested with a dedicated PSU rather than an admin account. An admin holding
CanUseAccountFirehoseAtAnyBankshort-circuitsAPIUtil.hasAccountAccessbefore consent is consulted, so every scoping result under such a user is meaningless.